Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests: add --db-backend pytest option #6625

Merged
merged 9 commits into from
Nov 25, 2024
Merged

tests: add --db-backend pytest option #6625

merged 9 commits into from
Nov 25, 2024

Conversation

danielhollas
Copy link
Collaborator

@danielhollas danielhollas commented Nov 20, 2024 •
edited
Loading

Pair coded and discussed with @GeigerJ2 🫶

Instead of choosing the database backend (SQLite or Postgres) implicitly via markers, we add an explicit --db-backend option to pytest.

  --db-backend=DB_BACKEND
                        Database backend to be used for tests ('sqlite', 'psql')

Important

The default backend was changed from psql to sqlite to make local testing easier.
The tests running in CI are still using Postgres in most places.
The only change in CI should be switching the rabbitmq tests and release tests to use sqlite.

Here's the error if the user provides an invalid option

❯ pytest --db-backend=invalid
ERROR: Invalid --db-backend option 'invalid'
Must be one of: ('sqlite', 'psql')

EDIT: To clarify, the existing presto marker works as before.

@codecov Codecov
Copy link

codecov bot commented Nov 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.90%. Comparing base (ef60b66) to head (a22f08b).
Report is 136 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6625      +/-   ##
==========================================
+ Coverage   77.51%   77.90%   +0.40%     
==========================================
  Files         560      567       +7     
  Lines       41444    42147     +703     
==========================================
+ Hits        32120    32831     +711     
+ Misses       9324     9316       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@danielhollas danielhollas marked this pull request as ready for review November 21, 2024 16:58
danielhollas
danielhollas commented Nov 21, 2024
View reviewed changes
.github/workflows/benchmark.yml Outdated
@@ -73,4 +73,4 @@ jobs:
alert-threshold: 200%
comment-on-alert: true
fail-on-alert: false
alert-comment-cc-users: '@chrisjsewell,@giovannipizzi'
alert-comment-cc-users: '@giovannipizzi,@agoscinski,@GeigerJ2'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if this benchmark alert works, somebody should test it :-)

danielhollas
danielhollas commented Nov 21, 2024
View reviewed changes
tests/test_markers.py Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests should perhaps live in test_conftest.py

GeigerJ2
GeigerJ2 reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@GeigerJ2 GeigerJ2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just went through a first time, but my brain is mush at this point. Will take it for another spin tomorrow. Just for clarification, because I'm still rather new to this whole CI business: Now, with the PSQL installation removed from the GH workflows, how/when are tests ever ran with PSQL (and I assume by default then only the ones marked with requires_psql).

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/ci-code.yml Outdated Show resolved Hide resolved
.github/workflows/nightly.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/test-install.yml Outdated Show resolved Hide resolved
.github/workflows/tests_nightly.sh Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
GeigerJ2
GeigerJ2 approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@GeigerJ2 GeigerJ2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this feature, @danielhollas, very nice. LGTM from my side!

@GeigerJ2 GeigerJ2 requested a review from unkcpz November 22, 2024 10:51
unkcpz
unkcpz approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also give a late review for the PR. Neat test feature!

@danielhollas
Copy link
Collaborator Author

Thanks for the reviews!

(btw: I don't have commit rights to this repo so please merge when you want)

@danielhollas danielhollas mentioned this pull request Nov 23, 2024
Merged
GeigerJ2 added a commit to unkcpz/aiida-core that referenced this pull request Nov 25, 2024
@GeigerJ2
Remove remaining pytest -v to mirror changes of aiidateam#6625.
fdf3fd2
@GeigerJ2 GeigerJ2 merged commit a863d1e into aiidateam:main Nov 25, 2024
32 of 33 checks passed
@GeigerJ2
Copy link
Contributor

Thanks for the great work, @danielhollas! I just squash-merged this, using a slightly modified version of your PR description as the commit message. Hope that's OK for you!

@danielhollas danielhollas deleted the pytest-db-option branch November 25, 2024 10:26
@danielhollas
Copy link
Collaborator Author

Hope that's OK for you!

Of course! Thanks for taking this over the finish line, I appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeigerJ2 GeigerJ2 GeigerJ2 approved these changes

@unkcpz unkcpz unkcpz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danielhollas @GeigerJ2 @unkcpz